dyncfg: require a ParameterScope on every Config - #38180
Conversation
antiguru
left a comment
There was a problem hiding this comment.
We should ensure that the optimizer overrides are scoped to their cluster.
| let replica_dyncfg = mz_dyncfgs::all_dyncfgs(); | ||
| ConfigUpdates::from(dyncfg).apply(&replica_dyncfg); |
There was a problem hiding this comment.
Why do we specialize here instead of within the replica task, or where it's created?
There was a problem hiding this comment.
Agreed, moved into the replica task in b23e77a.
ReplicaTask now owns a replica_dyncfg next to the environment-wide dyncfg, seeded at task creation and kept current by apply_config_command in the message loop, right where specialize_command already runs. SequentialHydration takes it as an argument to absorb_command / observe_response and holds no configuration of its own, so it cannot read the environment-wide value by accident. It reads the replica's or nothing.
Not at creation time, though: the task would then need the overrides plumbed into ReplicaClient::spawn as a second channel for something the command stream already carries, and it would go stale on the next push. Tracking the commands keeps one source of truth, and it is what the replica itself sees.
The test moved with the logic. replica_dyncfg_tracks_config_commands covers the seed, the CreateInstance snapshot, the UpdateConfiguration delta and the environment-wide set staying untouched. The interceptor's test is now behavioral rather than a getter check: at concurrency 1 the second Schedule is held back, and raising the concurrency in the supplied set releases it.
Generated by Claude Code
| self.replica_dyncfg_overrides = overrides | ||
| .values() | ||
| .flat_map(|replicas| replicas.iter()) | ||
| .map(|(replica_id, updates)| (*replica_id, updates.clone())) | ||
| .collect(); |
There was a problem hiding this comment.
Could this leak updates for replicas that are dropped?
There was a problem hiding this comment.
Yes. Fixed in b23e77a.
The assignment replaces the whole map, so it does not accumulate across pushes, and I had assumed that was enough. It is not: the coordinator only calls push_replica_dyncfg_overrides when replica_scoped_config_changed, so dropping a replica does not rebuild the map. A dropped replica's entry survives until the next scoped-config change, and an environment that churns replicas while the scoped config sits still accumulates them indefinitely.
Now pruned in the drop path instead of relying on the coordinator to re-push: Controller::drop_replica, ComputeController::drop_replica, compute Instance::remove_replica and storage Instance::drop_replica, which is all four places holding the map.
No correctness consequence that I can find, since replica IDs come from a monotonic sequence and are not reused, so a stale entry could never be picked up by a later replica. It was memory retention only.
Generated by Claude Code
| /// Environment-scoped because it is read twice, once by the optimizer in | ||
| /// `environmentd` and once at render time on the replica, and the two reads must | ||
| /// agree. It also changes what a subscribe emits, which replicas of one cluster | ||
| /// may not disagree about. |
There was a problem hiding this comment.
What happens if they don't agree?
There was a problem hiding this comment.
Nothing bad, which means my justification was wrong. Rewritten in b23e77a.
The two reads are asymmetric. In environmentd the flag gates whether optimize_dataflow_snapshot runs, which may clear with_snapshot on an import. On the replica, render.rs:279 computes if import.with_snapshot || !subscribe_snapshot_optimization { Include } else { Exclude }, so the replica-side read can only ever put a snapshot back, never take one away.
So both directions are safe. Flag on in environmentd and off on the replica means the snapshot is fetched anyway, correct but unoptimized. Flag off in environmentd and on at the replica means with_snapshot is still true, so the snapshot is included. Two replicas of one cluster disagreeing costs work, not correctness, since elision only happens where the optimizer proved the snapshot is not needed for the sink's output.
The real reason it is environment-scoped is the plan-time read: OptimizerConfig::from(&SystemVars) has no replica in scope, and no cluster either. Cluster would be the honest scope, but this is a plain OptimizerConfig field rather than an OptimizerFeature, and OptimizerFeatureOverrides is the only thing plan sites resolve cluster overrides through. persist_fast_path_order on the line above is in exactly the same position. Happy to file a follow-up for extending cluster resolution to the non-feature fields of OptimizerConfig if you think it is worth it.
Generated by Claude Code
|
On the review summary, about the optimizer overrides being scoped to their cluster. I checked every plan site, and the cluster-scoped optimizer overrides are resolved everywhere a cluster is in scope. Two gaps worth naming, neither introduced by this PR:
Dyncfgs cannot be cluster-scoped at all today. That is why this PR annotates 276 dyncfgs as 184 replica and 92 environment, with zero cluster. Cluster resolution at plan time runs entirely through Generated by Claude Code |
d94c317 to
64f5011
Compare
DAlperin
left a comment
There was a problem hiding this comment.
So, I've convinced myself the mechanism here is right. I wonder if we shouldn't get area owners to sanity check the scope for their flags though
martykulma
left a comment
There was a problem hiding this comment.
Thanks @antiguru ! A few of the source configurations should be environment - one of them is used by the dataflow and purification, and the others affect the emission of definite errors.
Other than that S&S lgtm!
| Duration::from_secs(30), | ||
| "Maximum amount of time we'll wait for SQL Server to report an LSN (in other words for \ | ||
| CDC to be fully enabled)", | ||
| ParameterScope::Replica, |
There was a problem hiding this comment.
This flag affects the behavior of both the source dataflow and purification, best make it Environment.
There was a problem hiding this comment.
Changed to Environment in 925f650, and confirmed the purification read you're describing: mz_sql::pure reads it at pure.rs:1030, pure.rs:1614 and pure.rs:1964, alongside the dataflow read at sql_server/replication.rs:234. The purification read happens in environmentd with no replica in scope, so replica-local was wrong.
Added a note on the declaration recording that, so the environment scope does not later look like an oversight on a config that is also read on clusterd.
Generated by Claude Code
| "pg_source_validate_timeline", | ||
| true, | ||
| "Whether to treat a timeline switch as a definite error", | ||
| ParameterScope::Replica, |
There was a problem hiding this comment.
this should be scoped to the environment.
There was a problem hiding this comment.
Done in 925f650, along with sql_server_source_validate_restore_history below and kafka_low_watermark_check.
These three are the same rule and I had it written on ParameterScope without applying it here: a config whose value must agree across the replicas of a cluster stays environment-wide even when it is read on clusterd. Deciding whether a definite error is emitted is exactly that, since the error lands in the collection, so replicas disagreeing would write different contents rather than merely doing different amounts of work. Each declaration now says so.
One I left alone and would like your read on: pg_schema_validation_interval, immediately above these, is still Replica. It controls how often validation runs rather than whether the error is emitted, so both replicas still eventually emit it, but it does influence when a definite error is written. If that timing needs to be coherent too, it should move with the others.
Generated by Claude Code
| "sql_server_source_validate_restore_history", | ||
| true, | ||
| "Whether to treat a restore history change as a definite error", | ||
| ParameterScope::Replica, |
There was a problem hiding this comment.
ditto for this one, should be environment scoped
| true, | ||
| "Whether to check the low watermark for Kafka sources and error if the start \ | ||
| offset/resume upper has been compacted away.", | ||
| ParameterScope::Replica, |
There was a problem hiding this comment.
this should be scoped to Environment
64f5011 to
925f650
Compare
|
Following up on @DAlperin's suggestion that area owners sanity-check their own flags, and @martykulma having done exactly that for Sources and Sinks. The scope of a config follows from where its value is realized, so these annotations are only as good as my reading of each config's read sites. A wrong one is invisible: the sync loop evaluates a context that nothing consumes, so a targeting rule silently does nothing, with no error and no failing test. Worth a few minutes from each owner. Where they live
Sources and Sinks is done. The other five are unreviewed by their owners. What to look for, in rough order of payoff
The rule I applied is written up on One thing that changed under this PR#38206 removed the Nothing here blocks merge from my side. If an area turns out to want changes, they are one-line edits to the declaration. Generated by Claude Code |
aljoscha
left a comment
There was a problem hiding this comment.
Looks good from my side: one thought I had is whether we actually want to allow the persist configs to be per-replica. And many of the persist configs also affect the persist code running on envd, so does that work well together?
The scope specifier was an optional `.scoped(..)` builder step, so a new dyncfg silently defaulted to environment-wide and a config that is actually realized per replica was easy to leave unannotated. An unannotated replica-local config is a silent no-op for the scoped feature flags layer: LaunchDarkly never evaluates a replica context for it, so a rule targeting a replica or size family does nothing. Make `scope` the fourth argument of `Config::new` and drop `Config::scoped`, so every declaration has to make the choice, then annotate all 276 existing dyncfgs from their read sites: 183 replica-local, 93 environment-wide. The rule, written down on `ParameterScope`: a config is replica-local when its value is realized inside a `clusterd` process. That covers the compute and storage worker config sets, the persist client config set, and `mz_metrics`, all four of which the per-replica dyncfg push reaches. `environmentd` reading such a config for its own process is fine, and sees the environment-wide value. A config realized in `environmentd` is environment-wide even when its effect concerns one replica, and a config whose value must agree across the replicas of a cluster stays environment-wide even when it is read on `clusterd`. Some configs are read in `environmentd` but shipped to a specific replica, which is the case that needs overrides constructed before the read. Those are now resolved against the replica's scoped overrides: * `Controller::provision_replica` builds a replica's `TimelyConfig` from `arrangement_exert_proportionality`, `enable_timely_zero_copy`, `enable_timely_zero_copy_lgalloc` and `timely_zero_copy_limit`. * `ComputeController::add_replica_to_instance` freezes `compute_replica_expiration_offset` and `enable_arrangement_dictionary_compression_alpha` into `ReplicaConfig`. To serve them, `Config::get_with_overrides` layers a replica's `ConfigUpdates` over a `ConfigSet`, the outer `Controller` keeps the pushed override map and fans it out to the compute and storage controllers, and the coordinator installs the map before creating replicas at bootstrap rather than only after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
The hydration limit is enforced in environmentd, by the controller's per-replica interceptor withholding `Schedule` commands, so declaring it replica-local only means something if the interceptor resolves its replica's override. Give `SequentialHydration` a config set of its own and feed it the `CreateInstance` and `UpdateConfiguration` commands it already absorbs. Those have been specialized for the replica by `Instance::specialize_command_for_replica`, so the set holds exactly what the replica itself reads. It has to be a set of its own rather than the controller's: a cloned `ConfigSet` shares its values, so applying the overrides to a clone would overwrite the environment-wide configuration for everyone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
Name the coarsest targeting granularity, not the process. balancerd's configs are Environment because balancerd resolves them against its own regional LaunchDarkly context, with nothing finer beneath it, not because balancerd is lumped in with environmentd. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
The replica task is where per-replica command specialization already lives, and its configuration is not the hydration interceptor's private business. Give `ReplicaTask` a `replica_dyncfg` holding what its replica reads, kept current from the configuration commands passing through, and hand it to `SequentialHydration` on each call. The interceptor now holds no configuration of its own, so it cannot read the environment-wide value by accident. It reads the replica's or nothing. Prune per-replica dyncfg overrides when a replica is dropped. The coordinator re-pushes the override map only when the scoped configuration itself changes, so a dropped replica's entry was otherwise retained until the next such change, and an environment churning replicas accumulated them. Correct the justification on `compute_subscribe_snapshot_optimization`. The two reads need not agree. The replica-side read only ever puts a snapshot back, so a disagreement costs work rather than correctness. It is environment-scoped because the plan-time read has no replica in scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
A config that decides whether a definite error is emitted has to be coherent across the replicas of a cluster: the error lands in the collection, so replicas disagreeing would write different contents. That covers kafka_low_watermark_check, pg_source_validate_timeline and sql_server_source_validate_restore_history. sql_server_max_lsn_wait is environment-wide for a different reason. Source purification reads it in environmentd as well as the source dataflow on the replica, and the purification read has no replica in scope. Each carries a note on why an environment-scoped config is read on clusterd, so it does not read as an oversight. Also annotate two configs main added since the signature changed: subscribe_max_buffered_bytes, read on the coordinator loop, and enable_compute_error_distinct, whose doc already recorded that replicas rendering under different values write the same thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
Persist configs were replica-local because the persist client config set runs on clusterd and the per-replica dyncfg push reaches it. That misses that the same client code also runs in environmentd, against the same shards, and no replica scope can reach that copy. A rollout targeting replicas would leave a shard's other writer on the old value indefinitely, so the per-replica capability is one that could never be exercised uniformly. Persist config also decides what lands in shared durable state, such as part sizes, encoding and compaction inputs. Per-replica divergence there means one shard written under several configurations depending on which process wrote, which is hard to attribute when something goes wrong. Declare the class environment-wide, and record the rule on ParameterScope and in the persist cfg module rather than on each of the 89 declarations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
…safe A finer scope enables divergence, so declaring one asserts that divergence is both safe and useful. Being wrong that way adds a way to break an environment that did not previously exist, while erring toward the environment scope only forgoes a capability. Make that asymmetry the governing rule on ParameterScope, with realization site a necessary rather than sufficient condition, and re-audit the annotations against it. Keep Replica for configs tuning a replica process's own resource usage that cannot change what a dataflow produces, what reaches durable state, or anything externally visible: lgalloc, the memory limiter, the spill and pager knobs, timely zero-copy, backpressure, hydration concurrency, metrics scrape intervals, source client timing and snapshot parallelism. Move the rest to Environment. Flags selecting a different implementation whose output-equivalence is an assumption rather than a guarantee, since a per-replica rollout would turn one bug into query results differing by which replica served them. Anything reaching durable state, including the MV sink paths, the upsert state representation and the persist-backed peek stash that environmentd also reads. Anything externally visible, including Kafka production and topic configuration, the S3 copy layout knobs and the SQL Server change-table cleanup that deletes from the upstream database. And ore_overflowing_behavior, which sets process-global overflow behavior and is read in environmentd too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU
925f650 to
0082a05
Compare
|
@aljoscha your persist question turned into a rethink of the whole PR, so this supersedes the by-area table I posted above. Replica-local went from 94 to 61, environment-wide from 188 to 221. PersistBoth of your points land, and the second is the decisive one. There is no scope meaning "every persist client in this environment". All 89 replica-local persist configs are now The principle behind it, applied to everything elseGeneralizing your point: So realization site is now a necessary condition for a finer scope, not a sufficient one, and re-auditing against that moved 33 more configs:
@martykulma this also answers the one I left open for you. What stays replica-local61 configs, one coherent class: @DAlperin the per-area sanity check is easier now: Persist and The rule is written up on Generated by Claude Code |
|
Every scope that changes in this PR, diffed against 52 configs change scope, and all 52 go (The re-audit I described earlier moved configs from Compute (24)
Replica provisioning,
|
|
Thanks for your reviews! I merged this in a more conservative form than what it was in between, so other than the code changes to add the explicit scope, not much should've shifted. I filed some Linear issues to capture potential follow-up work. |
…#38448) #38180 made the scope a required argument of `Config::new`. It landed after #38077's last CI run and before its squash merge, so `main` currently fails to compile `mz-persist` (five E0061 errors in `hedge.rs`, [test build 132566](https://buildkite.com/materialize/test/builds/132566)). The hedge configs are `Environment`-scoped like every other persist config, since the same client code runs in `environmentd` and `clusterd`. No functional change. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Motivation
The scope specifier introduced by the scoped feature flags work
(
doc/developer/design/20260609_scoped_feature_flags.md) was an optional.scoped(..)builder step onConfig. A new dyncfg therefore defaultedsilently to environment-wide, and a config that is actually realized per replica
was easy to leave unannotated. That failure mode is invisible: the LaunchDarkly
sync loop never evaluates a replica context for an unannotated config, so a rule
targeting a replica or a size family is a no-op with no error anywhere.
Only 11 dyncfgs carried a scope.
Description
Make
scopethe fourth argument ofConfig::newand dropConfig::scoped, soevery declaration has to make the choice, then annotate all 282 dyncfgs:
61 replica-local, 221 environment-wide, none cluster-coherent (no dyncfg
feeds
OptimizerFeatures, which is the only place a cluster-scoped value isresolved today).
The rule
Environmentis the safe declaration and the right one when in doubt. Itpreserves the unscoped behavior of a single value everywhere. A finer scope
enables divergence, so declaring one asserts that divergence is both safe and
useful for that config. Getting that wrong introduces a way to break an
environment that did not previously exist, while erring toward
Environmentonly forgoes a capability.
So where a config is realized is a necessary condition for a finer scope, not
a sufficient one. A config is declared
Replicaonly if it is realized perreplica and it tunes that replica process's own resource usage (memory, CPU,
I/O, concurrency, timing) and cannot change what a dataflow produces, what
reaches durable state, or anything externally visible.
The 61 that qualify are one class:
lgalloc, the memory limiter, the spill andpager knobs, timely zero-copy, logical backpressure, hydration concurrency,
metrics scrape intervals, source client timeouts and snapshot parallelism. Size
and shape tuning, which is the use case the design doc cites.
Everything else is
Environment, including:assumption rather than a guarantee (the join and render paths, temporal
bucketing, the correction buffer, the upsert state representation). Where the
assumption holds,
Environmentcosts nothing. Where it does not, a per-replicarollout turns one bug into query results that differ by which replica served
them.
paths and the persist-backed peek stash.
the S3
COPY TOlayout knobs, and the SQL Server change-table cleanup, whichdeletes rows from the upstream database.
clusterd, but the same client code also runs inenvironmentdagainst thesame shards, and no replica scope can reach that copy. A rollout targeting
replicas would leave a shard's other writer on the old value indefinitely, so
the per-replica capability is one that could never be exercised uniformly.
Recorded in
persist-client/src/cfg.rsrather than on each declaration.balancerd's configs.balancerdsyncs LaunchDarkly itself against abalancercontext keyed by provider, region and build version, with noenvironment, cluster or replica beneath it.
Environmentnames the coarsesttargeting granularity, not the
environmentdprocess.clusterd. Flags deciding whether a definite error is emitted are thesharp case: the error lands in the collection, so replicas disagreeing write
different contents rather than merely doing different amounts of work.
The full rule is written up on
ParameterScopeinsrc/dyncfg/src/lib.rs.environmentd read sites, where the overrides have to be constructed first
Seven configs are read in
environmentdbut resolved per replica. They now readthrough that replica's overrides, which is what makes their
Replicadeclaration real rather than decorative:
Controller::provision_replicabuilds a replica'sTimelyConfigfromarrangement_exert_proportionality,enable_timely_zero_copy,enable_timely_zero_copy_lgallocandtimely_zero_copy_limit.ComputeController::add_replica_to_instancefreezescompute_replica_expiration_offsetandenable_arrangement_dictionary_compression_alphaintoReplicaConfig.compute_hydration_concurrency, described below.Supporting changes:
Config::get_with_overrideslayers a replica'sConfigUpdatesover aConfigSet(a mistyped override is logged and ignoredrather than panicking a controller path); the outer
Controllerkeeps the pushedoverride map and fans it out to the compute and storage controllers, so the
coordinator has a single push entry point; and the coordinator installs the map
before creating replicas during bootstrap, not only after, since the
provisioning-time values are frozen by then.
The replica's effective configuration
ReplicaTaskowns areplica_dyncfgalongside the controller's environment-widedyncfg. It is seeded from the environment-wide set and then kept current fromthe
CreateInstancesnapshot and eachUpdateConfigurationdelta passingthrough the task, which
Instance::specialize_command_for_replicahas alreadyspecialized for this replica. It therefore holds exactly what the replica itself
reads.
It has to be a set of its own rather than a clone of the controller's: a cloned
ConfigSetshares its values, so applying one replica's overrides to a clonewould overwrite the environment-wide configuration for everyone.
SequentialHydrationis handed that set per call and holds no configuration ofits own, so it cannot read the environment-wide value by accident. This is the
shape worth repeating. Rather than documenting which set to read, arrange for
the wrong one not to be in scope.
Per-replica override lifetime
The coordinator re-pushes the override map only when the scoped configuration
itself changes, so dropping a replica does not rebuild it. The four holders of
the map (
Controller::drop_replica,ComputeController::drop_replica, computeInstance::remove_replica, storageInstance::drop_replica) now prune thedropped replica themselves. Memory retention only: replica IDs come from a
monotonic sequence and are not reused, so a stale entry could never be picked up
by a later replica.
Verification
cargo check -p mz-environmentd -p mz-clusterd -p mz-balancerd --all-targetsis clean across all three binaries,
bin/fmtapplied.mz-dyncfgunit test forget_with_overrides: absent override, presentoverride, and a type-mismatched override falling back to the set's value.
mz-compute-clientunit tests.replica_dyncfg_tracks_config_commandscovers the seed, the
CreateInstancesnapshot, theUpdateConfigurationdelta, and the environment-wide set staying untouched.
hydration_concurrency_follows_supplied_configcovers the interceptorbehaviorally: at a concurrency of one the second
Scheduleis held back, andraising the concurrency in the supplied set releases it.
Config::newcall's balanced argument listconfirms all 282 declarations carry a scope. Two further audits are clean: no
environment-scoped config is read from a
clusterd-side crate except thedocumented deliberate cases, and every replica-scoped config has a
clusterd-side read or is one of the seven resolved inenvironmentdabove.Note on risk: #38206 removed the
enable_scoped_system_parametersgate, soscoped evaluation is always on and these annotations take effect as soon as a
targeting rule exists. An earlier revision of this description claimed the
change was inert until that flag was enabled.
Not covered by an automated test: the controller wiring for the six
provisioning-time and replica-creation-time configs. Exercising it needs a
ComputeController/ orchestrator harness that does not exist today, so it isverified by construction and by the
get_with_overridestest underneath it.🤖 Generated with Claude Code
https://claude.ai/code/session_01RZo8dwEyXbXwUq6wb7BkeU